Skip to content

fix: don't prompt for creds for local models #567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

g-linville
Copy link
Member

@g-linville g-linville commented Jun 26, 2024

for #509

Details in comments.

var env map[string]string
if err := json.Unmarshal([]byte(authCfg.Password), &env); err != nil {
return Credential{}, err
if authCfg.Password != "" {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug where we would try to unmarshal an empty string.

Comment on lines +77 to 87
toolURL, err := url.Parse(toolName)
if err != nil {
return nil, err
}

// Save the path so we can put it back after removing it.
path := toolURL.Path
toolURL.Path = ""

toolName = toolURL.String() + ":" + possiblePortNumber + path
ctx = strings.TrimSuffix(ctx, ":"+possiblePortNumber)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug where we were putting the port after the path.
I.e., the string we're trying to fix is http://localhost/v1///default:5555 and we used to make it become http://localhost/v1:5555 rather than http://localhost:5555/v1.

@@ -108,7 +108,7 @@ func (c *Client) clientFromURL(ctx context.Context, apiURL string) (*openai.Clie
env := "GPTSCRIPT_PROVIDER_" + env2.ToEnvLike(parsed.Hostname()) + "_API_KEY"
key := os.Getenv(env)

if key == "" {
if key == "" && !isLocalhost(apiURL) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we no longer look for the credential in the store if the model is running locally. Not sure whether we care to support IPv6 (::1) here, but I can add that if we do.

Comment on lines +287 to +289
if alias != "" && !isAlphaNumeric(alias) {
return "", "", nil, fmt.Errorf("credential alias must be alphanumeric")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to add a constraint that a credential alias needs to be alphanumeric. This is to avoid interfering with credentials for model providers and such.

@g-linville g-linville marked this pull request as ready for review June 26, 2024 19:45
Signed-off-by: Grant Linville <[email protected]>
@g-linville g-linville requested a review from njhale June 26, 2024 19:54
@g-linville g-linville merged commit bcd9f33 into gptscript-ai:main Jun 27, 2024
8 checks passed
@g-linville g-linville deleted the fix-local-prompting branch June 27, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants